Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update HLS installation method #533

Merged
merged 52 commits into from
Mar 13, 2022
Merged

Conversation

hasufell
Copy link
Member

src/extension.ts Outdated Show resolved Hide resolved
@hasufell hasufell marked this pull request as draft February 28, 2022 01:01
@jneira
Copy link
Member

jneira commented Feb 28, 2022

sorry, don't have time to review the pr, hope another maintainer could do it

src/hlsBinaries.ts Outdated Show resolved Hide resolved
src/extension.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments for now

src/hlsBinaries.ts Outdated Show resolved Hide resolved
src/hlsBinaries.ts Outdated Show resolved Hide resolved
@hasufell hasufell marked this pull request as ready for review February 28, 2022 12:06
@hasufell
Copy link
Member Author

I'm quite pleased. We now:

  1. have our completely independent <storagePath>/.ghcup prefix where we manage HLS versions
  2. we can easily remove old versions, update etc.
  3. we could theoretically use this to pre-install cabal/ghc/stack as well

For the future, the extension could also have an option for the user to use the system-installed ghcup. But this really needs to be behind a config option imo.

@fendor
Copy link
Collaborator

fendor commented Feb 28, 2022

For the future, the extension could also have an option for the user to use the system-installed ghcup. But this really needs to be behind a config option imo.

I am interested in that, and I agree, it should be hidden behind a config option.

@hasufell
Copy link
Member Author

Some tests are failing, probably because we started messing with process.env:

2 failing

  1) Extension Test Suite
       Extension log should have server output:
     AssertionError [ERR_ASSERTION]: Extension log file has no hls output
  	at /home/runner/work/vscode-haskell/vscode-haskell/out/test/suite/extension.test.js:136:16
  	at Generator.next (<anonymous>)
  	at fulfilled (out/test/suite/extension.test.js:5:58)


  2) Extension Test Suite
       Server should inherit environment variables defined in the settings:

      AssertionError [ERR_ASSERTION]: Server did not inherit XDG_CACHE_DIR from environment variables set in the settings
      + expected - actual

      -false
      +true
      
  	at /home/runner/work/vscode-haskell/vscode-haskell/out/test/suite/extension.test.js:140:16
  	at Generator.next (<anonymous>)
  	at fulfilled (out/test/suite/extension.test.js:5:58)

Ideas?

@hasufell
Copy link
Member Author

hasufell commented Feb 28, 2022

I'm assuming the extension used to try to find older HLS releases if the current project GHC is too old?

@michaelpj
Copy link
Contributor

I'm assuming the extension used to try to find older HLS releases if the current project GHC is too old?

Yes, and this was a highly requested feature that we should definitely not break.

src/hlsBinaries.ts Outdated Show resolved Hide resolved
@hasufell
Copy link
Member Author

hasufell commented Mar 1, 2022

I'm assuming the extension used to try to find older HLS releases if the current project GHC is too old?

Yes, and this was a highly requested feature that we should definitely not break.

This is fixed in cda68ba

Example:
Screenshot_2022-03-01_00-58-33

@hasufell hasufell force-pushed the update-hls-installation branch 2 times, most recently from 1ff5f5b to 0bd3d76 Compare March 1, 2022 00:27
@hasufell
Copy link
Member Author

hasufell commented Mar 1, 2022

I don't understand why the tests are failing. I hope someone else can pick that up, because I'm running out of free time for this.

@hasufell hasufell force-pushed the update-hls-installation branch 4 times, most recently from 66a7f1e to c3aa0e7 Compare March 1, 2022 12:53
@hasufell
Copy link
Member Author

hasufell commented Mar 1, 2022

Well, I'm giving up on the test suite. It works locally. That's enough for me.

@fendor
Copy link
Collaborator

fendor commented Mar 1, 2022

Ill take a look

@hasufell hasufell force-pushed the update-hls-installation branch 2 times, most recently from cde83b8 to 86ba23c Compare March 2, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants